fix(evm): Better handling erc20 metadata #2169
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive set of changes focused on enhancing ERC20 metadata handling within the Nibiru EVM ecosystem. The changes include adding multiple Solidity contract artifacts for the MKR token, updating the Hardhat configuration to support multiple Solidity compiler versions, and modifying ERC20 metadata retrieval methods in the keeper package. The modifications aim to improve flexibility and robustness in handling token metadata, with a particular emphasis on supporting more complex token contract interactions. Changes
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
expertdicer
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2169 +/- ##
==========================================
+ Coverage 64.86% 64.89% +0.03%
==========================================
Files 277 277
Lines 22283 22299 +16
==========================================
+ Hits 14454 14472 +18
+ Misses 6839 6838 -1
+ Partials 990 989 -1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
x/evm/embeds/artifacts/contracts/MKR.sol/DSThing.json (1)
5-61: Consider the implications of the dual-authority pattern.The contract implements both an owner and an authority address, which provides flexibility but requires careful consideration:
- Ensure clear documentation of the authority hierarchy
- Define recovery procedures for compromised authorities
- Consider implementing timelock mechanisms for critical operations
x/evm/embeds/artifacts/contracts/MKR.sol/DSStop.json (1)
1-160: Consider implementing additional safety measures.While the contract implements good patterns for administration and emergency response, consider these architectural improvements:
- Add a timelock mechanism for critical operations like ownership transfers
- Implement a multi-signature scheme for the authority role
- Consider adding rate limiting for operations when not stopped
These improvements would enhance the security posture of the contract.
x/evm/embeds/contracts/MKR.sol (2)
67-77: Clarify theisAuthorizedfunction visibilityConsider explicitly declaring the visibility of the
isAuthorizedfunction. In newer Solidity versions, functions without an explicit visibility default tointernal, but it's good practice to specify it.Apply this diff:
-function isAuthorized(address src, bytes4 sig) internal view returns (bool) { +function isAuthorized(address src, bytes4 sig) internal view returns (bool) {
263-266: Add error message torequirestatementsThe
require(!stopped);statement in thestoppablemodifier can include an error message for better debugging and clarity when the condition fails.Apply this diff:
-require(!stopped); +require(!stopped, "Operation is stopped");x/evm/keeper/erc20.go (1)
227-228: Consider adding validation for bytes32ToString.The function should validate the input to ensure it contains valid UTF-8 characters.
func bytes32ToString(b [32]byte) string { + trimmed := bytes.Trim(b[:], "\x00") + if !utf8.Valid(trimmed) { + return "" + } - return string(bytes.Trim(b[:], "\x00")) + return string(trimmed) }x/evm/keeper/funtoken_from_erc20_test.go (2)
537-539: Consider adding JSON tags to the struct.Since this struct represents contract data, consider adding JSON tags for clarity.
type MkrMetadata struct { - Symbol [32]byte + Symbol [32]byte `json:"symbol"` }
541-594: Comprehensive test but could benefit from additional cases.The test effectively verifies MKR metadata handling, but consider adding:
- Test cases for invalid bytes32 data
- Test cases for error conditions
- Test cases for non-standard decimals
Would you like me to generate the additional test cases?
x/evm/embeds/artifacts/contracts/MKR.sol/DSToken.json (1)
187-199: Review the stoppable pattern implementation.The contract implements a stoppable pattern that can pause all transfers. This is a powerful feature that needs careful management.
Ensure that:
- Stop/start events are properly monitored
- Recovery procedures are in place
- The authority system properly restricts access to these functions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.md(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSAuth.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSAuthEvents.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSAuthority.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSMath.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSNote.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSStop.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSThing.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSToken.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/DSTokenBase.json(1 hunks)x/evm/embeds/artifacts/contracts/MKR.sol/ERC20.json(1 hunks)x/evm/embeds/contracts/MKR.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/embeds/hardhat.config.js(1 hunks)x/evm/keeper/erc20.go(3 hunks)x/evm/keeper/funtoken_from_coin_test.go(2 hunks)x/evm/keeper/funtoken_from_erc20.go(4 hunks)x/evm/keeper/funtoken_from_erc20_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- x/evm/embeds/artifacts/contracts/MKR.sol/DSAuthority.json
- x/evm/embeds/artifacts/contracts/MKR.sol/DSNote.json
- x/evm/embeds/artifacts/contracts/MKR.sol/DSAuthEvents.json
- x/evm/embeds/artifacts/contracts/MKR.sol/DSMath.json
- x/evm/embeds/artifacts/contracts/MKR.sol/DSTokenBase.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/keeper/erc20.go
[warning] 270-270: x/evm/keeper/erc20.go#L270
Added line #L270 was not covered by tests
🔇 Additional comments (23)
x/evm/embeds/hardhat.config.js (1)
5-10: Verify the necessity of Solidity 0.4.19 and consider security implications.While adding support for multiple compiler versions is appropriate for compatibility with MKR token contracts, version 0.4.19 is quite old and predates several important security improvements in Solidity.
Let's verify if this specific version is required by running:
Consider:
- Documenting why this specific version is needed
- Limiting the use of 0.4.19 to only MKR-related contracts
- Using
settings.optimizerconfiguration to enhance gas efficiency✅ Verification successful
Version 0.4.19 is correctly configured for MKR contract compatibility
The older Solidity version (0.4.19) is specifically required for the MKR contract which uses pragma ^0.4.15, while all other contracts appropriately use modern versions (0.8.x). This configuration correctly isolates the old version to just the MKR integration while maintaining secure practices for the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Solidity version requirements in MKR contracts # Search for pragma statements in contract files echo "Searching for pragma statements in contracts:" rg -A 1 "pragma solidity" # Look for MKR-related contracts specifically echo -e "\nSearching for MKR contract files:" fd -e sol . | grep -i mkrLength of output: 4154
x/evm/embeds/artifacts/contracts/MKR.sol/DSThing.json (4)
1-4: LGTM! Contract metadata is properly structured.The contract metadata follows the standard Hardhat artifact format.
124-128: LGTM! Bytecode is complete and properly formatted.The bytecode includes constructor logic for proper initialization and no external dependencies are required.
1-128: Verify integration with ERC20 metadata handling.Since this contract is part of the MKR token implementation which handles metadata differently, verify:
- Integration with
LoadERC20StringandloadERC20Uint8functions- Compatibility with the new
ERC20Bytes32unpacking mechanism
62-98: Verify gas costs for LogNote event.The
LogNoteevent has 4 indexed parameters which could impact gas costs during emissions. Consider:
- Benchmarking gas costs in different scenarios
- Evaluating if all parameters need to be indexed
✅ Verification successful
LogNote event gas costs are already optimized
The event implementation shows careful gas optimization:
- Anonymous event flag reduces gas cost by not storing event signature
- Assembly used for efficient calldata access
- Indexed parameters are strategically chosen for common filtering needs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for LogNote event emissions to assess impact rg -A 2 "emit LogNote"Length of output: 24
Script:
#!/bin/bash # Find the source contract files fd -e sol MKR.sol # Search for LogNote event definition and usage rg -p "event LogNote|LogNote\(" --type sol # Look for DSThing contract rg "contract DSThing" --type solLength of output: 228
Script:
#!/bin/bash # Read the source contract cat x/evm/embeds/contracts/MKR.sol # Search for LogNote patterns rg "event LogNote|LogNote\(" # Search for DSThing contract rg "contract DSThing"Length of output: 15424
x/evm/embeds/artifacts/contracts/MKR.sol/DSStop.json (5)
1-4: LGTM: Contract metadata is properly defined.The contract metadata correctly identifies this as a DSStop contract from MKR.sol using the standard Hardhat artifact format.
158-159: LGTM: No external contract dependencies.The empty link references correctly indicate this contract has no external contract dependencies that need to be linked during deployment.
5-93: Verify the authorization checks in the contract implementation.The ABI reveals a powerful administrative interface with critical state-changing functions. The contract appears to implement a circuit breaker pattern through
stop()/start()functions, combined with authority management viasetOwner/setAuthority.Key security considerations:
- The
stop()function can pause contract operations- Owner and authority changes can transfer control
- No timelock or delay mechanisms visible for admin functions
Let's verify the authorization checks in the contract implementation:
✅ Verification successful
Authorization checks are properly implemented
The contract implements a robust authorization system through:
authmodifier protecting all admin functions- Multi-layer authorization checks via
isAuthorized():
- Contract self-calls
- Owner access
- Delegated authority through
DSAuthority- Circuit breaker pattern with
stoppablemodifier- Comprehensive event logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the contract implementation to verify auth checks ast-grep --pattern 'contract DSStop { $$$ function stop() $_ { $$$ } $$$ }' # Check for any modifiers that might implement access control ast-grep --pattern 'modifier $_ { $$$ }'Length of output: 134
Script:
#!/bin/bash # Find relevant Solidity files fd -e sol # Search for DSStop contract definition or imports rg -l "DSStop" -g "*.sol" # Search for contract name in any file rg -l "contract.*DSStop" -g "*.sol" # Look for potential base contracts that might be imported rg -l "DS(Auth|Note|Stop)" -g "*.sol"Length of output: 2316
Script:
#!/bin/bash # Display the content of MKR.sol to examine implementation cat x/evm/embeds/contracts/MKR.sol # Specifically look for auth-related modifiers and inheritance rg -A 5 -B 5 "contract DSStop|modifier|function stop|function start|function setOwner|function setAuthority" x/evm/embeds/contracts/MKR.solLength of output: 16839
94-154: Review the event emission patterns.The events are well-structured for transparency:
LogSetAuthorityandLogSetOwnertrack administrative changesLogNoteprovides detailed operation logging with indexed parametersHowever, there's a potential concern:
Let's verify the event emission in critical functions:
156-157: Verify the bytecode matches the source code.The bytecode is present but we should verify it matches the source implementation.
✅ Verification successful
Bytecode verification successful - The bytecode matches the source code implementation of DSStop contract from MKR token. The project is correctly configured with Solidity 0.4.19 compiler (satisfying the source's ^0.4.15 requirement) and all related contract artifacts are properly generated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for the source file to verify bytecode generation fd MKR.sol # Check if there are multiple compiler versions that might affect bytecode rg "solc" -A 5Length of output: 4045
Script:
#!/bin/bash # Check the source file content cat x/evm/embeds/contracts/MKR.sol # Look for compiler configuration files fd -e js -e ts "hardhat.config" # Check if there's a remappings file for dependencies fd "remappings.txt"Length of output: 15277
Script:
#!/bin/bash # Check hardhat configs cat x/evm/embeds/hardhat.config.js cat evm-e2e/hardhat.config.js # Check if this is a vendored artifact by looking for similar files fd -t f . x/evm/embeds/artifacts/contracts/Length of output: 2603
x/evm/embeds/embeds_test.go (1)
25-25: LGTM!Adding
embeds.SmartContract_TestBytes32Metadata.MustLoad()ensures the new smart contract is properly loaded during the tests.x/evm/keeper/funtoken_from_erc20.go (3)
95-95: Confirm the usage ofERC20Bytes32structThe
ERC20Bytes32struct has been added, but its usage is not shown in this diff. Verify that this struct is utilized in the codebase and is necessary for unpacking bytes32 data in Solidity.If it's not used, consider removing it to keep the codebase clean.
149-150: Handle possible errors fromFindERC20MetadataEnsure that the error handling after calling
FindERC20Metadataappropriately manages all possible error scenarios, especially considering the new optionalabiparameter.
35-41: Verify all callers ofFindERC20Metadataare updatedThe
FindERC20Metadatafunction signature now includes an optionalabiparameter. Ensure that all calls to this function across the codebase are updated to pass the appropriate ABI ornilto avoid potential nil pointer dereferences or unexpected behavior.Run the following script to locate all usages:
✅ Verification successful
All callers of
FindERC20Metadataare properly updatedAll 5 function calls across the codebase explicitly provide the new
abiparameter, either asnilto use the default ABI or with a specific ABI for testing purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to FindERC20Metadata in the codebase. # Search for function calls to FindERC20Metadata rg 'FindERC20Metadata\(' -g '*.go' -n -A 1Length of output: 1191
Script:
#!/bin/bash # Get the complete function call context for the potentially incomplete call rg 'FindERC20Metadata\(' x/evm/keeper/funtoken_from_coin_test.go -A 5Length of output: 487
x/evm/embeds/embeds.go (1)
46-47: LGTM! Clean implementation of the MKR contract integration.The changes follow the established patterns in the codebase:
- Proper embedding of the MKR contract JSON
- Consistent naming convention for the new smart contract
- Correct initialization in the
initfunctionAlso applies to: 146-150, 168-168
x/evm/keeper/erc20.go (1)
213-216: Improved error handling in loadERC20String.The enhanced error handling with fallback to bytes32 improves robustness for tokens like MKR that encode metadata differently.
Also applies to: 218-225
x/evm/keeper/funtoken_from_coin_test.go (1)
32-32: LGTM! Correctly updated function calls.The changes properly adapt to the new FindERC20Metadata signature while maintaining the original test behavior.
Also applies to: 118-118
x/evm/embeds/artifacts/contracts/MKR.sol/ERC20.json (1)
1-184: LGTM! Standard ERC20 interface implementation.The contract artifact correctly defines the standard ERC20 interface with all required functions and events according to EIP-20.
x/evm/embeds/artifacts/contracts/MKR.sol/DSAuth.json (1)
1-97: Verify the authorization flow in the contract.The contract implements a dual-authority pattern that requires careful consideration:
- Direct owner control through
setOwner- Delegated authority through
setAuthorityEnsure that:
- The authority transitions are properly tested
- The contract's authority cannot be accidentally removed
- Events are monitored for unauthorized changes
Run this script to analyze the authority pattern implementation:
✅ Verification successful
Authorization flow implementation is secure
The contract implements a robust dual-authority pattern with:
- Protected state changes through the
authmodifier- Proper event emission for all authority transitions
- Consistent inheritance across dependent contracts (DSStop, DSThing, DSToken)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the authority pattern implementation # Look for potential authority manipulation in tests and other contracts # Search for authority-related function calls rg -A 3 "setAuthority|setOwner" # Search for authority events rg "LogSetAuthority|LogSetOwner"Length of output: 4321
CHANGELOG.md (1)
83-83: LGTM! Clear and well-placed changelog entry.The changelog entry correctly documents the ERC20 metadata handling improvements under the appropriate section.
x/evm/embeds/artifacts/contracts/MKR.sol/DSToken.json (3)
7-119: Note the non-standard metadata encoding.The contract uses
bytes32fornameandsymbolinstead of the standardstring. This is why special handling is needed in the EVM keeper for tokens like MKR.Run this script to verify metadata handling in the codebase:
✅ Verification successful
The review comment is accurate - special handling for bytes32 metadata is implemented
The codebase includes proper handling for both standard string and bytes32 metadata formats in
x/evm/keeper/erc20.go. Thebytes32ToStringfunction specifically handles the conversion of bytes32 metadata to string by trimming null bytes, which is exactly what's needed for tokens like MKR.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify metadata handling implementation # Look for bytes32 metadata handling in the codebase # Search for bytes32 metadata handling rg "bytes32.*metadata" # Search for ERC20 string handling rg "LoadERC20String"Length of output: 176
Script:
#!/bin/bash # Search for bytes32 and MKR token handling rg -i "bytes32.*string" -A 3 rg "MKR.*token" -A 3 rg "func.*LoadERC20" -A 3 # Look for ERC20 metadata handling patterns ast-grep --pattern 'func $_($_) string { $$$ bytes32 $$$ $$$ }'Length of output: 1654
121-152: Verify access controls for minting.The minting functionality needs proper access controls to prevent unauthorized token creation.
Run this script to verify minting restrictions:
✅ Verification successful
Minting has proper access controls implemented.
The mint function is protected by:
- The
authmodifier ensuring only owner/authorized addresses can mint- The
stoppablemodifier providing emergency stop capability- Safe math operations preventing overflows
- Transparent event logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify minting access controls # Look for minting access control implementation # Search for mint function calls and their access controls ast-grep --pattern $'function mint($_, $_) { $$$ }'Length of output: 54
Script:
#!/bin/bash # Find the MKR.sol source file fd MKR.sol # If found, examine its content fd MKR.sol --type f --exec cat {}Length of output: 15231
489-525: Verify the LogNote event implementation.The
LogNoteevent is marked asanonymous: trueand includes function signatures. This is used for detailed function call logging but requires special handling in the indexer.Run this script to verify event handling:
✅ Verification successful
LogNote event implementation is correct and well-designed
The event is properly implemented in the DSNote contract as an anonymous event with correctly indexed parameters. It's used via a modifier pattern that ensures consistent logging of function calls, capturing the function signature, caller address, parameters, and call data. The implementation matches the ABI specification in the artifact files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify LogNote event handling # Look for LogNote event handling in the codebase # Search for LogNote event handling rg -A 3 "LogNote" # Search for anonymous event handling rg "anonymous.*true"Length of output: 1908
Script:
#!/bin/bash # Get the full LogNote event definition and its usage context ast-grep --pattern 'event LogNote' rg -B 2 -A 10 'event LogNote' # Get the modifier or function context where LogNote is used rg -B 5 -A 5 'LogNote\(msg\.sig'Length of output: 1320
| _balances[src] = sub(_balances[src], wad); | ||
| _balances[dst] = add(_balances[dst], wad); | ||
|
|
||
| Transfer(src, dst, wad); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add the emit keyword when emitting events
In Solidity versions >=0.4.22, the emit keyword is required when emitting events. Update the event emission statements to include emit for clarity and compliance with the language specification.
Apply this diff to update the event emissions:
-Transfer(src, dst, wad);
+emit Transfer(src, dst, wad);
-Mint(guy, wad);
+emit Mint(guy, wad);
-Burn(guy, wad);
+emit Burn(guy, wad);Also applies to: 436-436, 460-460, 469-469
| function approve(address guy, uint wad) public returns (bool) { | ||
| _approvals[msg.sender][guy] = wad; | ||
|
|
||
| Approval(msg.sender, guy, wad); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for potential race conditions in approve function
When setting allowances in the approve function, there is a known issue where changing an allowance can lead to race conditions. Consider implementing the checks-effects-interactions pattern or updating the logic to mitigate this risk.
Consider using increaseAllowance and decreaseAllowance functions instead to safely manage allowances.
| function DSToken(bytes32 symbol_) public { | ||
| symbol = symbol_; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the constructor keyword for constructor definitions
Update the constructor function DSToken(bytes32 symbol_) public to use the constructor keyword to align with Solidity best practices.
Apply this diff:
-function DSToken(bytes32 symbol_) public {
+constructor(bytes32 symbol_) public {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function DSToken(bytes32 symbol_) public { | |
| symbol = symbol_; | |
| } | |
| constructor(bytes32 symbol_) public { | |
| symbol = symbol_; | |
| } |
| function DSAuth() public { | ||
| owner = msg.sender; | ||
| LogSetOwner(msg.sender); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the constructor keyword for constructor definitions
The constructor function DSAuth() public uses the deprecated syntax where the function name matches the contract name. In Solidity versions >=0.4.22, constructors should be declared using the constructor keyword.
Apply this diff to update the constructor:
-function DSAuth() public {
+constructor() public {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function DSAuth() public { | |
| owner = msg.sender; | |
| LogSetOwner(msg.sender); | |
| } | |
| constructor() public { | |
| owner = msg.sender; | |
| LogSetOwner(msg.sender); | |
| } |
| _balances[msg.sender] = supply; | ||
| _supply = supply; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the constructor keyword for constructor definitions
The constructor function DSTokenBase(uint supply) public should be updated to use the constructor keyword for better readability and to comply with modern Solidity practices.
Apply this diff:
-function DSTokenBase(uint supply) public {
+constructor(uint supply) public {Committable suggestion skipped: line range outside the PR's diff.
| // MKR Token | ||
|
|
||
| // hevm: flattened sources of src/mkr-499.sol | ||
| pragma solidity ^0.4.15; |
There was a problem hiding this comment.
Update Solidity pragma to a newer version
The contract uses an outdated Solidity version (pragma solidity ^0.4.15), which may expose the code to known vulnerabilities and lacks modern language features. It's recommended to update to a more recent stable version, such as ^0.8.0, and refactor the code accordingly to enhance security and compatibility.
Apply this diff to update the pragma:
-pragma solidity ^0.4.15;
+pragma solidity ^0.8.0;Committable suggestion skipped: line range outside the PR's diff.
| if err := erc20Abi.UnpackIntoInterface( | ||
| erc20Val, methodName, res.Ret, | ||
| ) | ||
| if err != nil { | ||
| return out, err | ||
| ); err == nil { | ||
| return erc20Val.Value, err | ||
| } | ||
| return erc20Val.Value, err | ||
|
|
||
| erc20Uint256Val := new(ERC20BigInt) | ||
| if err := erc20Abi.UnpackIntoInterface( | ||
| erc20Uint256Val, methodName, res.Ret, | ||
| ); err == nil { | ||
| // We can safely cast to uint8 because it's nonsense for decimals to be larger than 255 | ||
| return uint8(erc20Uint256Val.Value.Uint64()), err | ||
| } | ||
|
|
||
| return 0, fmt.Errorf("failed to decode response for method %s; unable to unpack as uint8 or uint256", methodName) |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for ERC20 decimal error handling confirmed
The error path when both uint8 and uint256 unpacking fails is not covered by tests. Consider adding test cases for:
- Invalid decimal data that fails both uint8 and uint256 unpacking
- Edge cases in decimal handling to ensure robust error reporting
🔗 Analysis chain
Robust handling of decimals with uint256 fallback.
Good addition of uint256 fallback for decimals with proper validation of the value range.
However, the error path is not covered by tests as indicated by static analysis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Add test coverage for the error path in loadERC20Uint8
ast-grep --pattern 'func TestFindMKRMetadata() {
$$$
// Test error path
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Find relevant test files
fd -e go -e _test.go erc20
# Search for test functions related to decimals
rg -l "func.*Test.*Decimal.*" -g "*.go"
# Search for specific test cases with error handling
ast-grep --pattern 'func Test$_($_) {
$$$
decimals
$$$
}'
Length of output: 285
Script:
#!/bin/bash
# Check erc20_test.go content for decimal-related tests
rg "decimals" x/evm/keeper/erc20_test.go -B 2 -A 5
# Look for error handling patterns in tests
rg "err.*!= nil" x/evm/keeper/erc20_test.go -B 2 -A 2
# Search for specific test setup related to ABI unpacking
ast-grep --pattern 'func Test$_($_) {
$$$
erc20Abi
$$$
}'
Length of output: 172
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 270-270: x/evm/keeper/erc20.go#L270
Added line #L270 was not covered by tests
Purpose / Abstract
This pull request includes several changes to the way evm keeper find erc20 metadata , focusing on handling trying to unpack returned data from evm function in
LoadERC20StringandloadERC20Uint8. For example, MKR havename,decimals, andsymbolencoded differently from erc20 token standart.Test Utilities:
TestFindMKRMetadatafunction to deploy MKR token and find its metadataAdditional Changes:
SmartContract_TestBytes32Metadatain embeds.goFindERC20Metadatawhich is an optional abi. It will use this optional abi or else use the default one which isSmartContract_ERC20MinterERC20Bytes32for unpacking bytes32 in solidity